Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix support for "redis.user" setting when authenticating to the Redis cache #2666

Merged
merged 10 commits into from
Feb 23, 2023

Conversation

NevilleS
Copy link
Contributor

@NevilleS NevilleS commented Feb 22, 2023

Closes #2665

Code Changes

  • Update cache.py client with optional username arg
  • Fix unexpected default Redis password: testpassword
  • Fix support for unauthenticated Redis servers

Steps to Confirm

I used my local Docker setup to test the following configs:

  • Redis server with no auth:
    • Fides with no auth (neither redis.password nor redis.user) connects
    • Fides with redis.password does not connect
    • Fides with redis.password and redis.user does not connect
  • Redis server with just password auth (e.g. redis-server --requirepass <password>):
    • Fides with correct redis.password and no redis.user connects
    • Fides with correct redis.password and incorrect redis.user does not connect
    • Fides with incorrect redis.password does not connect
    • Fides with no auth (neither redis.password nor redis.user) does not connect
  • Redis server with an ACL (e.g. redis-server <path/to/redis.conf>):
    • Fides with correct redis.password and redis.user connects
    • Fides with no auth (neither redis.password nor redis.user) does not connect
    • Fides with incorrect redis.password and redis.user does not connect

For testing the Redis server with an ACL, I created this file at ./docker/redis/redis.conf:

user redisadmin on ~* +@all >redispassword

I then modified docker-compose.yml to configure the redis service as:

  redis:
    image: "redis:6.2.5-alpine"
    command: redis-server /usr/local/etc/redis/redis.conf
    expose:
      - 6379
    ports:
      - "0.0.0.0:6379:6379"
    volumes:
      - type: bind
        source: ./docker/redis
        target: /usr/local/etc/redis
        read_only: False

And updated the .fides/fides.toml settings like:

[redis]
host = "redis"
user = "redisadmin"
password = "redispassword"
port = 6379
charset = "utf8"
default_ttl_seconds = 604800
db_index = 0
ssl = false
ssl_cert_reqs = "required"

Pre-Merge Checklist

@cypress
Copy link

cypress bot commented Feb 22, 2023

Passing run #314 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge e2ae769 into a2336a3...
Project: fides Commit: cad480d5ff ℹ️
Status: Passed Duration: 00:44 💡
Started: Feb 23, 2023 1:16 AM Ended: Feb 23, 2023 1:17 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Base: 86.39% // Head: 86.40% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e2ae769) compared to base (c35ce37).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2666   +/-   ##
=======================================
  Coverage   86.39%   86.40%           
=======================================
  Files         289      289           
  Lines       15993    16003   +10     
  Branches     2017     2018    +1     
=======================================
+ Hits        13817    13827   +10     
  Misses       1788     1788           
  Partials      388      388           
Impacted Files Coverage Δ
src/fides/api/ops/util/cache.py 93.96% <ø> (ø)
src/fides/core/config/redis_settings.py 85.00% <100.00%> (+2.14%) ⬆️
src/fides/api/main.py 79.01% <0.00%> (ø)
src/fides/api/ops/schemas/privacy_request.py 100.00% <0.00%> (ø)
src/fides/core/config/security_settings.py 98.78% <0.00%> (+0.01%) ⬆️
src/fides/api/ops/models/privacy_request.py 95.96% <0.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@NevilleS
Copy link
Contributor Author

When I configure my local Redis to enforce a username/password with an ACL, here's an example error I get:

fides-fides-1  | 2023-02-22 22:13:16.772 [ERROR] (main:setup_server:280): Connection to cache failed: WRONGPASS invalid username-password pair or user is disabled.

This tells me that I've clearly enabled the ACL correctly locally. Just doing a few more tests here but this feels good to go...

@NevilleS NevilleS marked this pull request as ready for review February 22, 2023 22:15
@NevilleS NevilleS changed the title Add support for redis.user for the cache Fix support for "redis.user" setting when authenticating to the Redis cache Feb 22, 2023
Copy link
Contributor Author

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments here - this got away from me a bit, but it should be good now.

.fides/fides.toml Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
src/fides/api/ops/util/cache.py Show resolved Hide resolved
src/fides/core/config/redis_settings.py Outdated Show resolved Hide resolved
src/fides/core/config/redis_settings.py Show resolved Hide resolved
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NevilleS

@NevilleS NevilleS merged commit e286cf7 into main Feb 23, 2023
@NevilleS NevilleS deleted the 2665-fix-redis-user branch February 23, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redis.user config setting is not used for cache connection
2 participants